Skip to content

fix compile error #895

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 8 commits into
base: develop
Choose a base branch
from
Open

fix compile error #895

wants to merge 8 commits into from
+13 −9

Conversation

Yancey2023
Copy link

@Yancey2023 Yancey2023 commented Jul 7, 2025 •
edited
Loading

Public API Changes

The xml parameter in UrdfModelOptions changed from type Element | undefined to Element | null

Description

fix a compile error in new version of xmldom:

src/urdf/UrdfModel.js:35:7 - error TS2322: Type 'Element | null' is not assignable to type 'Element | null | undefined'.
  Type 'Element' is missing the following properties from type 'Element': classList, className, clientHeight, clientLeft, and 103 more.

35       xmlDoc = parser.parseFromString(string, MIME_TYPE.XML_TEXT).documentElement;

@Yancey2023
Copy link
Author

Yancey2023 commented Jul 7, 2025 •
edited
Loading

To reproduce this compile error, clone this repo and run npm install.

MatthijsBurgh
MatthijsBurgh requested changes Jul 8, 2025
View reviewed changes
Copy link
Contributor

@MatthijsBurgh MatthijsBurgh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your change breaks the CI

@Yancey2023
Copy link
Author

Yancey2023 commented Jul 8, 2025 •
edited
Loading

Sorry about that. I only tested locally. I will try to fix it.

@Yancey2023
Copy link
Author

@MatthijsBurgh fixed

@MatthijsBurgh
Copy link
Contributor

@Yancey2023 Also make sure you update the lock file

@Yancey2023
Copy link
Author

Thanks for your review. I have update the lock file.

@Yancey2023 Yancey2023 requested a review from MatthijsBurgh July 8, 2025 18:51
@EzraBrooks
Copy link
Contributor

Ah, did we never add the typescript compiler to CI here? I thought we had. We should probably add that to prevent dependabot version bumps from breaking the build.

@EzraBrooks
Copy link
Contributor

Actually, I see that the Vite build is running in CI here.

roslibjs/test/build.bash

Line 28 in 2af39fa

npm run build

In what situation are you encountering this build issue? I am not seeing an issue in CI or on my local machine.

@Yancey2023
Copy link
Author

Yancey2023 commented Jul 9, 2025 •
edited
Loading

The issue arises because the dependency versions in package.json are prefixed with a caret (^), which means any version above this is acceptable. As a result, when my computer has newer versions of dependencies, it uses those updated dependencies. However, these newer versions may be incompatible with 'roslib', which can lead to compilation errors.

@Yancey2023
Copy link
Author

Initially, I wasn't know the reason. Therefore, the PR initially only fixed the compilation errors without updating the dependency versions. Since the CI's npm uses caching, it didn’t use the newest dependency versions, causing the CI to initially fail.

@Yancey2023
Copy link
Author

I think others have also encountered this issue: #838 (comment)

@Yancey2023
Copy link
Author

There is also a PR that attempts to fix this issue, but it does so in an wrong way: #870

MatthijsBurgh
MatthijsBurgh requested changes Jul 9, 2025
View reviewed changes
Copy link
Contributor

@MatthijsBurgh MatthijsBurgh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure whether we need all dependency bumps. Only bump dependencies of which the older version is not working anymore

@Yancey2023
Copy link
Author

Yancey2023 commented Jul 9, 2025 •
edited
Loading

I'm not sure it is a good idea because npm will try to install the newest version as default when version is prefixed with a caret (^) and the depencedy is not cached.

@Yancey2023
Copy link
Author

Hello. I can't understand why we're not using the newest version. Can you show me more details?

@Yancey2023 Yancey2023 requested a review from MatthijsBurgh July 10, 2025 16:22
@MatthijsBurgh
Copy link
Contributor

Dependencies tell what you need, the lock file tells what you use.
So if we don't need a newer version, don't bump it. Improves compatibility for dependents

@Yancey2023
Copy link
Author

Sorry for the late reply and my misunderstanding. Everything is done now and I hope you can feel free to review it.

@MatthijsBurgh
Copy link
Contributor

@Yancey2023 Please resolve the conflicts

@MatthijsBurgh MatthijsBurgh self-requested a review August 15, 2025 12:57
@Yancey2023
Copy link
Author

The conflicts is resolved so you can review now. By the way, I just notice that roslibjs was migrated to typescript and I think it's wonderful. I really love it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@EzraBrooks EzraBrooks Awaiting requested review from EzraBrooks

@sea-bass sea-bass Awaiting requested review from sea-bass

@MatthijsBurgh MatthijsBurgh Awaiting requested review from MatthijsBurgh

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Yancey2023 @MatthijsBurgh @EzraBrooks